-
Notifications
You must be signed in to change notification settings - Fork 264
Max/crates io prep v2 #6270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Max/crates io prep v2 #6270
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Cargo.toml
Outdated
|
|
||
| # sdk related deps so we can pull in workspace versions of these in other crates' cargo files, and not have to define the version everywhere | ||
| # nym-api = { version = "1.2.0", path = "nym-api" } | ||
| nym-bandwidth-controller = { version = "1.2.0", path = "common/bandwidth-controller" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure you have to specify the version on top here (plz double check), but if not, i'd remove it so we wouldn't cause some inconsistencies if we forget to update those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the hacky way I found which allows us to define the workspace.version for the constituent crates. Maybe I can get rid of those.. Will experiment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we have to keep the top level version - but cargo workspaces should keep it all in line
| [package] | ||
| name = "sqlx-pool-guard" | ||
| version = "0.1.0" | ||
| name = "nym-sqlx-pool-guard" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you've renamed this, after the PR gets merged, create a draft on the vpn repo to update their dependencies (as they were importing it too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to self - check where this occurs elsewhere Looks like that was the only spot.
PR created, in draft until this PR is merged.
d07d3ad to
712ad35
Compare
f7df17e to
aea7607
Compare
2766acc to
6e61430
Compare
| on: | ||
| workflow_dispatch: | ||
| inputs: | ||
| version: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this require a separate validation or cargo workspaces handles it properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding, crates.io would reject the upload if e.g. someone fat-fingered the release version to a previous version, or something like that.
What other validation were you thinking of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add validation to make the version is properly formatted etc
| prometheus = { version = "0.14.0" } | ||
|
|
||
| # Workspace dep definitions required by crates.io publication - we need a workspace version since `cargo workspaces` doesn't work with path imports from crate manifests | ||
| nym-api-requests = { version = "1.20.1", path = "nym-api/nym-api-requests" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you don't have to specify version in here, since workspace already has it and workspace crates take it from workspace.package.version anyway.
It's enough to leave path = .. in here and remove duplicate version definitions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need it for cargo workspaces, otherwise when we try and publish we get errors like this:
error: failed to verify manifest at `/home/m/dev/work/nym/common/cosmwasm-smart-contracts/coconut-dkg/Cargo.toml`
Caused by:
all dependencies must have a version requirement specified when publishing.
dependency `nym-contracts-common` does not specify a version
Note: The published dependency will use the version from crates.io,
the `path` specification will be removed from the dependency declaration.
This PR makes the necessary modifications to be able to publish the majority of the Nym monorepo workspace crates to https://crates.io (essentially everything aside from binaries, internal tools, and a few select other things).
Overall, this PR:
adds a script to usecargo releaseto publish the SDK + its dependencies -cargo releasedeals with creating them in the correct ordercargo.tomlfilecargo.tomlpathto using theworkspaceversionTodo:
gitcratesblscrate: upload fork to crates.ioto make sure that there is no issue with new clients/gws and legacy ones after the dependency upgrade. If all good, changeTesting has been done on a NymVPN instance that was compiled with the new dependency interacting with mainnet gateways. The reverse situation will be tested by QA.gitimport to0.8.0from crates as per PRexperiment: remove separateEDIT: publish crates then just use crates.io imports forcontracts/workspace - TEMP in max/crates-io-prep-v2-contract-experimentcontracts/nym-credential-verificationcompilation errs in dryrunpublishing.mdwriteupThis change is